Only fail on on relevant message topic in message count test #178
Conversation
rotu
changed the title
/string_topic messages
|
It seems I've fixed the same problem as #165. I recommend merging both :-) |
|
Isn't that defeating the purpose of the test though? The test name Why is there a different node still publishing though? The tests are written in a way that during execution no other nodes are active within the system. |
|
I think you're right. The test should record all topics. The '/parameter_events' topic for 'use_sim_time' is emitted by rclcpp itself. I'll take this back to the drawing board and figure out why. I think we should be filtering the messages after receiving them. It would be nice if this test didn't require strict isolation in order to pass. |
rotu
changed the title
Fixed this in #180. |
rotu
force-pushed
the
patch1
branch
2 times, most recently
from
252d9ae to
4b42d66
Compare
|
I don’t think it should be. They both seem like good refinements. |
|
fair enough. The current PR has some conflicts. Do you mind resolving them so that I can run CI on it? |
Karsten1987
left a comment
Karsten1987
left a comment
There was a problem hiding this comment.
This patch looks good to me. I'll have to wait until rebased and run CI on it.
| for (auto & msg : writer_->get_messages()) { | ||
| EXPECT_NE(msg->topic_name, topic); | ||
| } |
There was a problem hiding this comment.
I think it's easier to replace this for loop with a call to writer_->get_topics().
Something like:
auto recorded_topics = writer_->get_topics();
EXPECT_EQ(0u, recorded_topics.count(topic));
6ffe653 to
1184034
Compare
|
|
Sorry @rotu, the intraprocess PR just landed in the meantime. Do you mind rebasing one more time please? |
I found that this test was failing on CycloneDDS because some unrelated node was publishing on the `parameter_events` topic, and these were getting caught up in the bag, and failing the test. Signed-off-by: Dan Rose <[email protected]>
|
@Karsten1987 You got it! |
|
Karsten1987
left a comment
Karsten1987
left a comment
There was a problem hiding this comment.
thanks for the patch.
3 participants
I found that this test was failing on CycloneDDS because some unrelated node was publishing on the
parameter_eventstopic, and these were getting caught up in the bag, and failing the test.